Skip to content

OpenConceptLab/ocl_issues#2733 | GHA to create release, tag and generate changelog - #899

Open
snyaggarwal wants to merge 1 commit into
masterfrom
issues#2733
Open

snyaggarwal wants to merge 1 commit into
masterfrom
issues#2733

Conversation

@snyaggarwal

Copy link
Copy Markdown
Contributor

Linked Issue

Refs OpenConceptLab/ocl_issues#2733

@snyaggarwal
snyaggarwal requested a review from paynejd September 16, 2026 13:04
@snyaggarwal snyaggarwal self-assigned this Sep 16, 2026

@paynejd paynejd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as the canonical of the eight OpenConceptLab/ocl_issues#2733 PRs. I hashed the shared files across all eight: release_version.sh has exactly two variants differing only in the two config lines (core/__init__.py/API_VERSION vs package.json/version), and set_build_version.sh likewise -- so this review covers the script for the whole fleet, and each repo's build.yml is reviewed on its own PR.

The consolidation is a real improvement: one repo-agnostic script with a two-line config block beats eight divergent inline workflow blobs, is_prerelease is correct, and the --target ${GITHUB_SHA} pinning is right.

Two blockers, both about the output this produces rather than the structure.


R1 (blocking) -- the release is still titled with a commit hash, which is AC 1

OpenConceptLab/ocl_issues#2733 opens with: "Publicly released versions must be friendly, not a generic hash at the end like 3.0.0-alpha-0efa4b8d." That string is the ticket's own example of the thing to eliminate. This script produces exactly it:

tag="${current_version}-${sha}"     # line 71
gh release create "$tag" \
  --title "$tag" \                  # line 87

So the releases page still reads 3.0.0-alpha-0efa4b8d, and the primary acceptance criterion isn't met.

The sha is also redundant for uniqueness: cmd_publish bumps MINOR in VERSION_FILE and pushes that bump immediately after every successful release, so two consecutive releases can never carry the same version. Uniqueness is already guaranteed by the bump. Inline suggestion below drops the sha from both tag and title and keeps it in the notes, where it's still useful for tracing an artifact.

R2 (blocking) -- on five of the eight repos the first release note is the entire repo history

prev_tag="$(git describe --tags --abbrev=0 2>/dev/null || true)"
if [ -n "$prev_tag" ]; then
  changelog="$(git log "${prev_tag}..HEAD" ...)"
else
  changelog="$(git log --pretty=format:'- %s (%h)')"   # <- no range
fi

Tag counts on the eight target repos:

oclapi2             419 tags
oclweb2             232
oclweb3               4
oclmap                0   <- 795 commits
ocl-community-site    0   <- 152 commits
ocl-ai-assistant      0   <- 135 commits
ocl-analytics-api     0   <-  66 commits
ocl_online_api        0

Five have no tags at all, so their first release publishes the whole history as the changelog -- 795 lines for oclmap, which is the repo the Phase 1 preview points new users at. Either seed a baseline tag per repo before merging, or bound the fallback (git log -n 50, or from the repo's first commit only when that's small).


Should-fix

R3 -- the push retry loop reports success after failing.

for attempt in 1 2 3; do
  if git push origin "$default_branch"; then break; fi
  git fetch origin "$default_branch" --quiet
  git rebase "origin/${default_branch}"
done

If all three attempts fail the loop just ends, cmd_publish returns 0 and the job goes green -- but the version was never bumped, so the next run republishes the same version and gh release create fails on a duplicate tag. set -euo pipefail doesn't catch it because the failing git push is the condition of an if. Needs an explicit failure after the loop. A conflicting git rebase also leaves the working tree mid-rebase with no --abort, so attempts 2 and 3 are operating on a broken tree.

R4 -- no concurrency guard. None of the eight workflows declare concurrency. Two merges in quick succession both read the same current_version, both try to create that tag, and both try to push a bump. Worth adding:

concurrency:
  group: release-${{ github.ref }}
  cancel-in-progress: false

R5 -- default_branch="${GITHUB_REF_NAME:-main}". The fallback is wrong for oclapi2 and oclweb2, whose default branch is master. GitHub Actions always sets GITHUB_REF_NAME on a push trigger so it won't bite today, but a script whose documented fallback pushes to a branch that doesn't exist in half the fleet is worth fixing while it's cheap.

R6 -- cmd_current matches a substring. grep -m1 "$VERSION_KEY" "$VERSION_FILE" takes the first line containing the key, so any earlier package.json entry whose name contains version would win. Anchoring the grep ("\"version\":") removes the failure mode.

R7 -- permissions: contents: write is workflow-level, so every job in the run gets write on contents when only release needs it. There's a commented-out per-job block in build.yml suggesting this was already on your mind.


Not on this PR -- OpenConceptLab/ocl_issues#2733 itself

Two of the ticket's three criteria are written-policy deliverables:

  • "Written policy/description of version naming convention"
  • "Written policy of how each repo release will be described"

No PR in the fleet adds documentation, so the ticket can't close on these even once the code lands. The third criterion names oclweb3, oclapi2, oclmap as the key repos and this went to eight, which is fine -- but worth noting the scope grew. (The Mapper version issue from the 2026-09-02 addendum is already resolved: oclmap/main reads 1.0.0-beta.)

Comment thread release_version.sh
docker push $DOCKER_IMAGE_NAME:$TAG
current_version="$(cmd_current)"
sha="$(cmd_sha "${GITHUB_SHA:-}")"
tag="${current_version}-${sha}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1. This is where the ticket's primary AC is lost. The bump at the end of cmd_publish already guarantees consecutive releases carry different versions, so the sha adds nothing to uniqueness and reintroduces exactly the 3.0.0-alpha-0efa4b8d shape OpenConceptLab/ocl_issues#2733 asks to remove.

Suggested change
tag="${current_version}-${sha}"
tag="${current_version}"

and drop it from the title too (see the companion comment below), keeping the sha in the release notes for artifact tracing.

Comment thread release_version.sh
Comment on lines +85 to +89
gh release create "$tag" \
--target "${GITHUB_SHA:-HEAD}" \
--title "$tag" \
--notes "$changelog" \
"${prerelease_args[@]}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1 (cont). With tag no longer carrying the sha, record it in the notes instead so an artifact is still traceable to a commit:

Suggested change
gh release create "$tag" \
--target "${GITHUB_SHA:-HEAD}" \
--title "$tag" \
--notes "$changelog" \
"${prerelease_args[@]}"
gh release create "$tag" \
--target "${GITHUB_SHA:-HEAD}" \
--title "$tag" \
--notes "Build: ${sha}
${changelog}" \
"${prerelease_args[@]}"

Comment thread release_version.sh
Comment on lines +73 to +79
git fetch --tags --quiet || true
prev_tag="$(git describe --tags --abbrev=0 2>/dev/null || true)"
if [ -n "$prev_tag" ]; then
changelog="$(git log "${prev_tag}..HEAD" --pretty=format:'- %s (%h)')"
else
changelog="$(git log --pretty=format:'- %s (%h)')"
fi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R2. With no tags in the repo this else branch emits the entire history -- 795 commits for oclmap, 152 for ocl-community-site, 135 for ocl-ai-assistant, 66 for ocl-analytics-api. Bound it:

Suggested change
git fetch --tags --quiet || true
prev_tag="$(git describe --tags --abbrev=0 2>/dev/null || true)"
if [ -n "$prev_tag" ]; then
changelog="$(git log "${prev_tag}..HEAD" --pretty=format:'- %s (%h)')"
else
changelog="$(git log --pretty=format:'- %s (%h)')"
fi
git fetch --tags --quiet || true
prev_tag="$(git describe --tags --abbrev=0 2>/dev/null || true)"
if [ -n "$prev_tag" ]; then
changelog="$(git log "${prev_tag}..HEAD" --pretty=format:'- %s (%h)')"
else
# No tags yet: this is the repo's first release, so there is no "since last
# release" range. Cap it rather than dumping the entire history.
changelog="$(git log -n 50 --pretty=format:'- %s (%h)')"
fi

Seeding a baseline tag per repo before merging would work too, and gives a truthful first changelog rather than a truncated one.

Comment thread release_version.sh
Comment on lines +104 to +110
for attempt in 1 2 3; do
if git push origin "$default_branch"; then
break
fi
echo "Push rejected, rebasing and retrying (${attempt})..."
git fetch origin "$default_branch" --quiet
git rebase "origin/${default_branch}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R3. If all three pushes fail the loop exits and cmd_publish returns 0, so the job is green with no version bump -- and the next run then fails on a duplicate tag. A conflicting rebase also leaves the tree mid-rebase for the following attempts.

Suggested change
for attempt in 1 2 3; do
if git push origin "$default_branch"; then
break
fi
echo "Push rejected, rebasing and retrying (${attempt})..."
git fetch origin "$default_branch" --quiet
git rebase "origin/${default_branch}"
for attempt in 1 2 3; do
if git push origin "$default_branch"; then
break
fi
if [ "$attempt" = 3 ]; then
echo "Failed to push the version bump after 3 attempts" >&2
exit 1
fi
echo "Push rejected, rebasing and retrying (${attempt})..."
git fetch origin "$default_branch" --quiet
git rebase "origin/${default_branch}" || { git rebase --abort || true; exit 1; }
done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants